Reviewed 66 of 68 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 83 at r3 (raw file):

            cell.titleLabel.text = portString
            // TODO: replace this with a tagged AccessibilityIdentifier

Is this planned work in an upcoming pr? Otherwise we should either do it here or remove the todo.

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

            // location here is fine.
            if let location = item.node.locations.first {
                // we can probably replace this with a tagged AccessibilityIdentifier and cut this case statement

If you have an idea, go for it :)

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

        // Ensure changelog is no longer shown
        _ = app
            .otherElements[AccessibilityIdentifier.changeLogAlert.asString]

Thanks to the subscript function in XCUIElementQuery we can simplify this with .otherElements[.changeLogAlert]. This apply to all similar cases below.

Please make sure to also run the UITests on the CI to guarantee we don't break anything (or at least not make it worse given its recent issues)

Reviewed 66 of 68 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv)

ios/MullvadVPN/View controllers/CreationAccount/Welcome/WelcomeContentView.swift line 254 at r3 (raw file):

    @objc private func tapped(button: AppButton) {
        switch button.accessibilityIdentifier {
        case AccessibilityIdentifier.purchaseButton.asString:

Do we need to specify the whole type name here ? It feels like a downgrade in readability

ios/MullvadVPN/Classes/AccessbilityIdentifier.swift line 218 at r3 (raw file):

extension AccessibilityIdentifier {
    public var asString: String {

Let's just make it compliant to CustomStringConvertible instead of doing this, we don't need to reinvent the wheel

Reviewable status: 66 of 69 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/Classes/AccessbilityIdentifier.swift line 218 at r3 (raw file):

That's what I did initially. The problem is, if you do that, you enter a loop, with "\(self)" calling the newly available self.description method until the stack overflows. There does not appear to be a user-accessible tagged-enum-formatting mechanism lower level than string interpolation (which calls description).

For what it's worth, the string we're getting is more specific than a catch-all description: it is a unique value generated from the enum and used for comparison, so it is not inappropriate for it to have a different computed variable.

ios/MullvadVPN/View controllers/CreationAccount/Welcome/WelcomeContentView.swift line 254 at r3 (raw file):

Unfortunately, we do, as we're doing string comparisons. Unless we somehow hack storage onto arbitrary UIKit objects to store our structured AccessibilityIdentifier type, or else implement a parser to convert arbitrary strings from UIKit to whatever they plausibly came from, we're working in the string domain. Though there isn't a huge amount of this code, and perhaps a future solution could be a Swift macro to hide the boilerplate.

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

I tried replacing that one with a AccessibilityIdentifier.locationCell(RelayLocation), though bringing MullvadTypes into the dependencies caused building UI tests to fail with Missing required module "WireGuardKitC". Is there an easy fix for this?

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 83 at r3 (raw file):

Done.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

I don't think so, although I'm not terribly confident in evaluating the dependency problems we have sometimes.

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

Not sure I understand what you mean. We don't need any extra handling for most (all?) of them, right? And if we do, perhaps we could just have make and exception for outliers.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

Ah yes, I see. Done.

Reviewable status: 67 of 69 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

Fair enough. I'll consign this particular case statement to the too-hard basket for now, removing this comment, and stick to attaching types that don't come from dependency hell.

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

Reviewed 3 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion